-
Notifications
You must be signed in to change notification settings - Fork 375
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(gno.land): add go type checking to keeper + tx simulation in gnokey #1702
Conversation
…no into dev/morgan/precompile-refactor
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1702 +/- ##
==========================================
- Coverage 54.95% 54.18% -0.78%
==========================================
Files 481 520 +39
Lines 67407 73038 +5631
==========================================
+ Hits 37044 39575 +2531
- Misses 27341 30245 +2904
- Partials 3022 3218 +196 ☔ View full report in Codecov by Sentry. |
With the latest commits, this PR additionally changes the behaviour of Essentially, when performing
cc/ @piux2 @jaekwon @moul for a thumbs up on the behaviour and API changes cc/ @leohhhn re: docs changes |
From Review Meeting:
|
@piux2 I've reverted the changes to Store, but I still modified the functioning of Store's import cycle detection to work using a slice rather than a map. When I had import cycles happen in real-life scenarios, I found the "dependency path" that Go showed me quite useful, as I could pinpoint not just what package was being recursively imported, but also which other package was importing it. This is possible only with slices, because maps don't guarantee the order of their keys. |
after a few hours of painful debugging; I remembered the other reason why I'd made the store changes. (cc/ @piux2) Consider this code: gno/gno.land/pkg/sdk/vm/builtins.go Lines 15 to 44 in a03eeb3
This is used to initialize the VM store: gno/gno.land/pkg/sdk/vm/keeper.go Lines 79 to 80 in a03eeb3
So, what happens in gno/gno.land/pkg/sdk/vm/keeper.go Lines 104 to 128 in a03eeb3
The problem is the following: at the time being, when the GnoVM fails to get a package, it will call the package getter... but it will always use the same store (in this case, Thus, this is actually problematic (I just didn't remember it, because it's been a while since I did the changes); when importing packages that don't exist in the store, the GnoVM will load things up from This created an issue in the CI: https://github.com/gnolang/gno/actions/runs/9022495035/job/24792161138 The "std" package was attempted to be resolved, but it was only being loaded in the top-level store, and not the fork, thus creating the "import not found". Which is why we need to change the signature of the PackageGetter, and make sure that it gets the up-to-date store and not pollute the global store. |
@jaekwon as we agreed last thursday, with the goal of unblocking further work depending on this I'm merging this PR I'm happy to re-discuss any further changes to be made if you still want to make a review |
Hi @thehowl . This PR is a welcome improvement, but it is effectively a breaking change. I am happily going through my realm and test code, fixing compiler errors and trying to get it to load. You may want to warn people. |
…key (gnolang#1702) Split from gnolang#1695 for ease of reviewing. Merge order: 1. gnolang#1700 2. gnolang#1702 (this one!) 3. gnolang#1695 \ gnolang#1730 This PR removes `TranspileAndCheckMempkg` in favour of performing the type checking it was supposed to do using `go/types` with a custom importer. This importer works together with Gno's `Store`, and can as such be used to type check Gno packages without ever writing a single file to disk. It is important to note that by "Go type check" I mean a variety of compile-time checks the Go compiler performs; in fact, this is much more powerful than running "gofmt" as we are currently doing. Additionally, it adds a new flag to gnokey, `-simulate`, to control transaction simulation before committing a transaction. See [this issue comment](gnolang#1702 (comment)) Resolves gnolang#1661. ## Reviewing notes - transpiler.TranspileAndCheckMempkg has been removed from the gnokey client and gnoclient, in favour of having this step be performed on the vm keeper. This paves the way for clients to not have to include the entire GnoVM, which I call a win. - Stdlib io had a precompiling error due to an unused variable (`remaining`); I updated it to the latest code on Go's standard libraries. - `Store` changes - `Store` has been changed to have its `getPackage` method work by detecting import cycles, without risking race conditions (the current implementation is not thread-safe). This is done by creating a new store, `importerStore`, which contains the previously imported paths in the current chain. Cyclic imports are still (correctly) detected in the tests. - `GetMemPackage` has been changed to return nil when a package cannot be found. This matches its behaviour with `GetMemFile`, which already did this when the file does not exist. - `GetMemPackage`, if a package is not found in the store, now attempts retrieving it using Store.GetPackage first. The underlying reason is that the Gno importer for the type checker needs to access the source of the standard libraries; however, these are never in any transaction and are not executed "per se" when the blockchain start. As a consequence, they may not exist within the Store; as a solution, when using GetMemPackage, we ensure that a package does not exist by checking if GetPackage does not retrieve it through getMemPackage and save it.
This PR adds a simple regression test for a bug that was introduced in gnolang#1702 and fixed in gnolang#2105 . Signed-off-by: Jeff Thompson <jeff@thefirst.org>
Merge order: 1. #1700 2. #1702 3. #1695 (this one!) -- review earlier ones first, if they're still open! This PR modifies the Gno transpiler (fka precompiler) to use Gno's standard libraries rather than Go's when performing transpilation. This creates the necessity to transpile Gno standard libraries, and as such support their native bindings. And it removes the necessity for a package like `stdshim`, and a mechanism like `stdlibWhitelist`. - Fixes #668. Fixes #1865. - Resolves #892. - Part of #814. - Makes #1475 / #1576 possible without using hacks like `stdshim`. cc/ @leohhhn @tbruyelle, as this relates to your work ## Why? - This PR enables us to perform Go type-checking across the board, and not use Go's standard libraries in transpiled code. This enables us to _properly support our own standard libraries_, such as `std` but any others we might want or need. - It also paves the way further to go full circle, and have Gno code be transpiled to Go, and then have "compilable" gno code ## Summary of changes - The transpiler has been thoroughly refactored. - The biggest change is described above: instead of maintaing the import paths like `"strconv"` and `"math"` the same (so using Gno's stdlibs in Gno, and Go's in Go), the import paths for standard libraries is now also updated to point to the Gno standard libraries. - Native functions are handled by removing their definitions when transpiling, and changing their call expressions where appropriate. This links the transpiled code directly to their native counterparts. - This removes the necessity for `stdlibWhitelist`. - As a consequence, `stdshim` is no longer needed and has been removed. - Test files are still not "strictly checked": they may reference stdlibs with no matching source, and will not be tested when running with `--gobuild`. This is because packages like `fmt` have no representation in Gno code; they only exist as injections in `tests/imports.go`. I'll fix this eventually :) - The CLI (`gno transpile`) has been changed to reflect the above changes. - Flag `--skip-fmt` has been removed (the result of transpile is always formatted, anyway), and `--gofmt-binary` too, obviously. `gno transpile` does not perform validation, but will gladly provide helpful validation with the `--gobuild` flag. - There is another PR that adds type checking in `gno lint`, without needing to run through the transpilation step first: #1730 - It now works by default by looking at "packages" rather than individual files. This is necessary so that when performing `transpile` on the `examples` directory, we can skip those where the gno.mod marks the module as draft. These modules make use of packages like "fmt", which because they don't have an underlying gno/go source, cannot be transpiled. - Running with `-gobuild` now handles more errors correctly; ie., all errors not previously captured by the `errorRe` which only matches those pertaining to a specific file/line. - `gnoFilesFromArgs` was unused and as such deleted - `gnomod`'s behaviour was slightly changed. - I am of the opinion that `gno mod download` should not precompile what it downloads; _especially_ to gather the dependencies it has. I've changed it so that it does a `OnlyImports` parse of the file it downloads to fetch additional dependencies Misc: - `Makefile` now contains a recipe to calculate the coverage for `gnovm/cmd/gno`, and also view it via the HTML interface. This is needed as it has a few extra steps (which @gfanton already previously added in the CI). - Realms r/demo/art/gnoface and r/x/manfred_outfmt have been marked as draft, as they depend on packages which are not actually present in the Gno standard libraries. - The transpiler now ignores draft packages by default. - `ReadMemPackage` now also considers Go files. This is meant to have on-chain the code for standard libraries like `std` which have native bindings. We still exclude Go code if it's not in a standard library. - `//go:build` constraints have been removed from standard libraries, as go files can only have one and we already add our own when transpiling ## Further improvements after this PR - Scope understanding in `transpiler` (so call expressions are not incorrectly rewritten) - Correctly transpile gno.mod --------- Co-authored-by: Antonio Navarro Perez <antnavper@gmail.com> Co-authored-by: Miloš Živković <milos.zivkovic@tendermint.com>
Also includes improved coloredbytes and debug printing, and more comments. At first I thought #1702's passing of the store was not ideal, but upon much confusion with cache invalidation, it became clear that passing in a store to getPackage() makes sense. This means that any store operations that occur through the loading of dependencies will incur gas charges for the transaction, e.g. for AddPkg() with dependencies like "time" or "strconv". Rather than clear cache-misses from the cacheStore, which is confusing, we would be better off passing in a mutated store go getPackage (if we need to). Or, just load the standard packages upon genesis. Also added improvements to ColoredBytes; this is now faster since not every character needs to be escaped, but rather escaping happens in chunks. As part of this refactor, the key & values are also clipped. I suppose we could maybe 1. improve ColoredBytesN() to clip exactly to N, but implementing this is non-trivial, and also 2. make the key/value limits perhps depend on a configuration or environment variable. Poll... would you be sad if the Print() output for databases clipped the values? I think it makes it much better for dev experience; and if you need the full value you can tinker with the source where appropriate. The downside is, we might lose information from logs. But I'm not sure we even use the Print() feature for any logs as of now.
Merge order: 1. gnolang#1700 2. gnolang#1702 3. gnolang#1695 (this one!) -- review earlier ones first, if they're still open! This PR modifies the Gno transpiler (fka precompiler) to use Gno's standard libraries rather than Go's when performing transpilation. This creates the necessity to transpile Gno standard libraries, and as such support their native bindings. And it removes the necessity for a package like `stdshim`, and a mechanism like `stdlibWhitelist`. - Fixes gnolang#668. Fixes gnolang#1865. - Resolves gnolang#892. - Part of gnolang#814. - Makes gnolang#1475 / gnolang#1576 possible without using hacks like `stdshim`. cc/ @leohhhn @tbruyelle, as this relates to your work ## Why? - This PR enables us to perform Go type-checking across the board, and not use Go's standard libraries in transpiled code. This enables us to _properly support our own standard libraries_, such as `std` but any others we might want or need. - It also paves the way further to go full circle, and have Gno code be transpiled to Go, and then have "compilable" gno code ## Summary of changes - The transpiler has been thoroughly refactored. - The biggest change is described above: instead of maintaing the import paths like `"strconv"` and `"math"` the same (so using Gno's stdlibs in Gno, and Go's in Go), the import paths for standard libraries is now also updated to point to the Gno standard libraries. - Native functions are handled by removing their definitions when transpiling, and changing their call expressions where appropriate. This links the transpiled code directly to their native counterparts. - This removes the necessity for `stdlibWhitelist`. - As a consequence, `stdshim` is no longer needed and has been removed. - Test files are still not "strictly checked": they may reference stdlibs with no matching source, and will not be tested when running with `--gobuild`. This is because packages like `fmt` have no representation in Gno code; they only exist as injections in `tests/imports.go`. I'll fix this eventually :) - The CLI (`gno transpile`) has been changed to reflect the above changes. - Flag `--skip-fmt` has been removed (the result of transpile is always formatted, anyway), and `--gofmt-binary` too, obviously. `gno transpile` does not perform validation, but will gladly provide helpful validation with the `--gobuild` flag. - There is another PR that adds type checking in `gno lint`, without needing to run through the transpilation step first: gnolang#1730 - It now works by default by looking at "packages" rather than individual files. This is necessary so that when performing `transpile` on the `examples` directory, we can skip those where the gno.mod marks the module as draft. These modules make use of packages like "fmt", which because they don't have an underlying gno/go source, cannot be transpiled. - Running with `-gobuild` now handles more errors correctly; ie., all errors not previously captured by the `errorRe` which only matches those pertaining to a specific file/line. - `gnoFilesFromArgs` was unused and as such deleted - `gnomod`'s behaviour was slightly changed. - I am of the opinion that `gno mod download` should not precompile what it downloads; _especially_ to gather the dependencies it has. I've changed it so that it does a `OnlyImports` parse of the file it downloads to fetch additional dependencies Misc: - `Makefile` now contains a recipe to calculate the coverage for `gnovm/cmd/gno`, and also view it via the HTML interface. This is needed as it has a few extra steps (which @gfanton already previously added in the CI). - Realms r/demo/art/gnoface and r/x/manfred_outfmt have been marked as draft, as they depend on packages which are not actually present in the Gno standard libraries. - The transpiler now ignores draft packages by default. - `ReadMemPackage` now also considers Go files. This is meant to have on-chain the code for standard libraries like `std` which have native bindings. We still exclude Go code if it's not in a standard library. - `//go:build` constraints have been removed from standard libraries, as go files can only have one and we already add our own when transpiling ## Further improvements after this PR - Scope understanding in `transpiler` (so call expressions are not incorrectly rewritten) - Correctly transpile gno.mod --------- Co-authored-by: Antonio Navarro Perez <antnavper@gmail.com> Co-authored-by: Miloš Živković <milos.zivkovic@tendermint.com>
Also includes improved coloredbytes and debug printing, and more comments. At first I thought gnolang#1702's passing of the store was not ideal, but upon much confusion with cache invalidation, it became clear that passing in a store to getPackage() makes sense. This means that any store operations that occur through the loading of dependencies will incur gas charges for the transaction, e.g. for AddPkg() with dependencies like "time" or "strconv". Rather than clear cache-misses from the cacheStore, which is confusing, we would be better off passing in a mutated store go getPackage (if we need to). Or, just load the standard packages upon genesis. Also added improvements to ColoredBytes; this is now faster since not every character needs to be escaped, but rather escaping happens in chunks. As part of this refactor, the key & values are also clipped. I suppose we could maybe 1. improve ColoredBytesN() to clip exactly to N, but implementing this is non-trivial, and also 2. make the key/value limits perhps depend on a configuration or environment variable. Poll... would you be sad if the Print() output for databases clipped the values? I think it makes it much better for dev experience; and if you need the full value you can tinker with the source where appropriate. The downside is, we might lose information from logs. But I'm not sure we even use the Print() feature for any logs as of now.
Split from #1695 for ease of reviewing. Merge order:
feat(cmd/gno): perform type checking when calling linter #1730
This PR removes
TranspileAndCheckMempkg
in favour of performing the type checking it was supposed to do usinggo/types
with a custom importer. This importer works together with Gno'sStore
, and can as such be used to type check Gno packages without ever writing a single file to disk. It is important to note that by "Go type check" I mean a variety of compile-time checks the Go compiler performs; in fact, this is much more powerful than running "gofmt" as we are currently doing.Additionally, it adds a new flag to gnokey,
-simulate
, to control transaction simulation before committing a transaction. See this issue commentResolves #1661.
Reviewing notes
remaining
); I updated it to the latest code on Go's standard libraries.Store
changesStore
has been changed to have itsgetPackage
method work by detecting import cycles, without risking race conditions (the current implementation is not thread-safe). This is done by creating a new store,importerStore
, which contains the previously imported paths in the current chain. Cyclic imports are still (correctly) detected in the tests.GetMemPackage
has been changed to return nil when a package cannot be found. This matches its behaviour withGetMemFile
, which already did this when the file does not exist.GetMemPackage
, if a package is not found in the store, now attempts retrieving it using Store.GetPackage first. The underlying reason is that the Gno importer for the type checker needs to access the source of the standard libraries; however, these are never in any transaction and are not executed "per se" when the blockchain start. As a consequence, they may not exist within the Store; as a solution, when using GetMemPackage, we ensure that a package does not exist by checking if GetPackage does not retrieve it through getMemPackage and save it.Questions
ValidateBasic()
would allow us to check that the code is invalid before the tx is added to the mempool (don't take my word on this.) Would that make sense? Related discussion on chore: add more validation toMsgRun
&MsgAddPackage
#1646.